Implement handling PreserveSig = false for interfaces #1206

kant2002 · 2021-06-07T05:57:10Z

This was done by pretending return value is out argument passed as last value.
We still need to return that value, so this require change in the Marshaller.cs so it would not throw assertion, since previously only Return values can return value.

For now only primitive types and Interface and Object are supported.
I opt out of DelegateMarshallingMethodThunk because when these classes created with PreserveSig = false. I assume this is because of laziness during creation of this class because this flag was not used previously. I'm not sure should I optout of DelegateMarshallingMethodThunk or just set PreserveSig flag?
I add special case for handling blittable types which are return type for PreserveSig=false methods. In this case I fallback to default marshalling implementation.

Closes #1183

This was done by pretending return value is out argument passed as last value. We still need to return that value, so this require change in the Marshaller.cs so it would not throw assertion, since previously only Return values can return value.

kant2002 · 2021-06-07T10:10:05Z

I opt out of DelegateMarshallingMethodThunk because when these classes created with PreserveSig = false. I assume this is because of laziness during creation of this class because this flag was not used previously. I'm not sure should I optout of DelegateMarshallingMethodThunk or just set PreserveSig flag?

… for PreserveSig=false methods. In this case I fallback to default marshalling implementation.

kant2002 · 2021-06-07T11:23:57Z

I add special case for handling blittable types which are return type for PreserveSig=false methods. In this case I fallback to default marshalling implementation.

jkotas · 2021-06-07T15:18:07Z

I'm not sure should I optout of DelegateMarshallingMethodThunk or just set PreserveSig flag?

It would be nice to make it work the same way as in CoreCLR.

jkotas · 2021-06-07T15:12:25Z

src/coreclr/tools/Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs

+            {
+                TypeDesc managedReturnType = _targetMethod.Signature.ReturnType;
+                bool supportedType = managedReturnType.IsPrimitive || managedReturnType.IsInterface || managedReturnType.IsObject;
+                if (!supportedType)


What prevents the other cases from being supported? I would expect the implementation to work for all marshalers without special casing.

It require more testing. Nothing in principle, just my inability to see all supported cases.
Also https://github.com/dotnet/runtime/blob/1843cdef72e1e7c810c4ea85c677d679ff3b66f4/src/coreclr/vm/dllimport.cpp#L3560-L3564 indicate cases which are not supported, but that does not give me much information what is supported. I think I can add Enum and String without probably much problems if needed.

All types that can be marshalled should be supported.

The unsupported cases in CoreCLR are just a legacy baggage from the times where the marshalling stubs were hand-emitted x86 assembly and handling them would require writing a pile of new hand-emitted x86 assembly. We should not need need to worry about artificially throwing exceptions for the cases that are unsupported in CoreCLR for no good reason.

I think that the transformation for PreserveSig = false should be very regular, it should depend on the regular [out] marshalling. It should not depend on the actual type at all.

[PreserveSig = false] RETVAL foo(...);

should be transformed to:

RETVAL foo(...) { RETVAL retNative = default; marshal everything usual actual_call(..., &retNative); unmarshal everything as usual (including retNative / retManaged) return retManaged; }

src/coreclr/tools/Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs

kant2002 · 2021-06-07T16:52:12Z

It would be nice to make it work the same way as in CoreCLR.

Reverse marshalling I do not fully grasp yet. I do not yet hit case where I was involved enough to implement.

jkotas · 2021-06-07T18:28:26Z

src/coreclr/tools/Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs

@@ -97,7 +97,9 @@ private static Marshaller[] InitializeMarshallers(MethodDesc targetMethod, Inter
                TypeDesc parameterType = (i == 0)


(i == 0 && isHRSwapping) condition is repeated multiple times here. Also, I do not think you should need the check for DelegateMarshallingMethodThunk here. We should make sure to set PreserveSig=true for marshalled delegates earlier (UnmanagedFunctionPointerAttribute won't allow you to specify PreserveSig=false so we do not need to worry about handling it).

I think it would be a bit more efficient and readable like this:

TypeDesc parameterType; bool isHRSwappedRetVal = false; if (i == 0) { //first item is the return type parameterType = methodSig.ReturnType; if (!flags.PreserveSig && !parameterType.IsVoid) { // PreserveSig = false can only show up an regular forward PInvokes Debug.Assert(direction == MarshalDirection.Forward); parameterType = methodSig.Context.GetByRefType(parameterType); isHRSwappedRetVal = true; } } else { parameterType = methodSig[i - 1]; }

jkotas · 2021-06-07T18:29:09Z

src/coreclr/tools/Common/TypeSystem/IL/Stubs/PInvokeILEmitter.cs

            ILEmitter emitter = ilCodeStreams.Emitter;
            ILCodeStream fnptrLoadStream = ilCodeStreams.FunctionPointerLoadStream;
            ILCodeStream callsiteSetupCodeStream = ilCodeStreams.CallsiteSetupCodeStream;
            TypeSystemContext context = _targetMethod.Context;

+            bool isHRSwapping = !_flags.PreserveSig && _targetMethod.Signature.ReturnType != _targetMethod.Context.GetWellKnownType(WellKnownType.Void)


== targetMethod.Context.GetWellKnownType(WellKnownType.Void) can be just IsVoid

jkotas · 2021-06-07T18:41:34Z

It would be nice to add one test for this to smoke tests. I think this is tricky enough to have it covered in the smoke tests.

jkotas

LGTM otherwise. Thank you!

jkotas · 2021-06-08T05:55:06Z

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs

@@ -512,7 +512,7 @@ protected virtual void EmitMarshalReturnValueManagedToNative()

        public virtual void LoadReturnValue(ILCodeStream codeStream)
        {
-            Debug.Assert(Return);
+            Debug.Assert(Return || Index == 0);


It would be nice to replace all Index == 0 checks with a property. It would make the code much easier to understand.

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs

jkotas · 2021-06-08T05:59:18Z

src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs

@@ -903,7 +912,7 @@ class BlittableValueMarshaller : Marshaller
    {
        protected override void EmitMarshalArgumentManagedToNative()
        {
-            if (IsNativeByRef && MarshalDirection == MarshalDirection.Forward)
+            if (IsNativeByRef && MarshalDirection == MarshalDirection.Forward && Index > 0)


I would move the special case first and then leave the rest of the method intact.

if (IsHRSwappedRetVal) { ...; return; }

Co-authored-by: Jan Kotas <[email protected]>

kant2002 added 2 commits June 7, 2021 11:54

Ignore delegating marshalling thunks

f614212

I add special case for handling blittable types which are return type…

15a5c68

… for PreserveSig=false methods. In this case I fallback to default marshalling implementation.

jkotas changed the title ~~Implement handling PerserveSig = false for interfaces~~ Implement handling PreserveSig = false for interfaces Jun 7, 2021

jkotas reviewed Jun 7, 2021

View reviewed changes

Remove artificial constraing on PreserveSig=false

48de7aa

jkotas reviewed Jun 7, 2021

View reviewed changes

kant2002 added 3 commits June 8, 2021 00:55

Address PR feedback

0b5bb1c

Fix

dbdb239

Add simple tests for PreservSig=false functionality

5903921

jkotas approved these changes Jun 8, 2021

View reviewed changes

kant2002 and others added 2 commits June 8, 2021 12:22

Update src/coreclr/tools/Common/TypeSystem/Interop/IL/Marshaller.cs

df9bdb1

Co-authored-by: Jan Kotas <[email protected]>

Reformat code based on PR feedback

5e098f6

kant2002 mentioned this pull request Jun 8, 2021

COM interop support #306

Closed

11 tasks

jkotas merged commit 2fe0c20 into dotnet:feature/NativeAOT Jun 8, 2021

kant2002 deleted the kant/preservesig-false branch June 8, 2021 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement handling PreserveSig = false for interfaces #1206

Implement handling PreserveSig = false for interfaces #1206

kant2002 commented Jun 7, 2021 •

edited

Loading

kant2002 commented Jun 7, 2021

kant2002 commented Jun 7, 2021

jkotas commented Jun 7, 2021

jkotas Jun 7, 2021

kant2002 Jun 7, 2021

jkotas Jun 7, 2021

kant2002 commented Jun 7, 2021

jkotas Jun 7, 2021

jkotas Jun 7, 2021

jkotas commented Jun 7, 2021 •

edited

Loading

jkotas left a comment

jkotas Jun 8, 2021

jkotas Jun 8, 2021

		@@ -97,7 +97,9 @@ private static Marshaller[] InitializeMarshallers(MethodDesc targetMethod, Inter
		TypeDesc parameterType = (i == 0)

Implement handling PreserveSig = false for interfaces #1206

Implement handling PreserveSig = false for interfaces #1206

Conversation

kant2002 commented Jun 7, 2021 • edited Loading

kant2002 commented Jun 7, 2021

kant2002 commented Jun 7, 2021

jkotas commented Jun 7, 2021

jkotas Jun 7, 2021

Choose a reason for hiding this comment

kant2002 Jun 7, 2021

Choose a reason for hiding this comment

jkotas Jun 7, 2021

Choose a reason for hiding this comment

kant2002 commented Jun 7, 2021

jkotas Jun 7, 2021

Choose a reason for hiding this comment

jkotas Jun 7, 2021

Choose a reason for hiding this comment

jkotas commented Jun 7, 2021 • edited Loading

jkotas left a comment

Choose a reason for hiding this comment

jkotas Jun 8, 2021

Choose a reason for hiding this comment

jkotas Jun 8, 2021

Choose a reason for hiding this comment

kant2002 commented Jun 7, 2021 •

edited

Loading

jkotas commented Jun 7, 2021 •

edited

Loading